-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): Implement 3ds decision manger for V2 #7022
base: main
Are you sure you want to change the base?
Conversation
let key_manager_state: &KeyManagerState = &(&state).into(); | ||
let profile_id = profile.get_id(); | ||
|
||
let profile = db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have the profile in the params, we need not fetch the profile again
created_at: timestamp, | ||
}; | ||
|
||
let business_profile_update = domain::ProfileUpdate::DecisionManagerRecordUpdate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to clone the decision_manager_record here?
.attach_printable("Failed to update routing algorithm ref in business profile")?; | ||
|
||
Ok(service_api::ApplicationResponse::Json( | ||
decision_manager_record, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the decision_manager_record from the profile, from profile update response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dashboard changes look fine.
#[derive(Serialize, Deserialize, Debug, Clone, FromSqlRow, AsExpression, ToSchema)] | ||
#[diesel(sql_type = Jsonb)] | ||
/// DecisionManagerRecord | ||
pub struct DecisionManagerRecord { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be having a struct with the same name in the api_models
crate as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have not reused the same struct as the one which is in api_models have a field called pub modified_at: i64,
but for V2 we don't need that field.
let updated_profile = db | ||
.update_profile_by_profile_id( | ||
key_manager_state, | ||
&key_store, | ||
profile, | ||
business_profile_update, | ||
) | ||
.await | ||
.change_context(errors::ApiErrorResponse::InternalServerError) | ||
.attach_printable("Failed to update routing algorithm ref in business profile")?; | ||
|
||
Ok(service_api::ApplicationResponse::Json( | ||
updated_profile | ||
.three_ds_decision_manager_config | ||
.clone() | ||
.get_required_value("three_ds_decision_manager_config") | ||
.change_context(errors::ApiErrorResponse::InternalServerError) | ||
.attach_printable("Failed to get updated routing algorithm ref in business profile")?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failed to get updated routing algorithm ref in business profile
Are these error messages correct here?
let record = profile | ||
.three_ds_decision_manager_config | ||
.clone() | ||
.ok_or_else(|| errors::ApiErrorResponse::InternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use ok_or()
itself here.
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Response
Response
Checklist
cargo +nightly fmt --all
cargo clippy